Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: cases to querystring related to empty string #11329

Closed
wants to merge 1 commit into from

Conversation

watilde
Copy link
Contributor

@watilde watilde commented Feb 12, 2017

Increase coverage of querystring:

This test case will cover these lines:

Together with #11326, the coverage will be 100%.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 12, 2017
@hiroppy hiroppy added the querystring Issues and PRs related to the built-in querystring module. label Feb 12, 2017
check(qs.parse(), {});

// empty sep
check(qs.parse('a', []), { a: '' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a semicolon to this line;)

check(qs.parse('a', []), { a: '' })

// empty eq
check(qs.parse('a', null, []), { '': 'a' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry for both of them... I've updated immediately.

@mscdex
Copy link
Contributor

mscdex commented Feb 12, 2017

@@ -232,8 +232,20 @@ assert.doesNotThrow(function() {
assert.strictEqual(f, 'a:b;q:x%3Ay%3By%3Az');
}

// empty string
assert.strictEqual('', qs.stringify());
Copy link
Member

@lpinca lpinca Feb 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can you please swap the parameters? actual first, then expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing!

+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty
jasnell pushed a commit that referenced this pull request Feb 16, 2017
+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty

PR-URL: #11329
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Feb 16, 2017

Landed in eec216f

@jasnell jasnell closed this Feb 16, 2017
@watilde watilde deleted the test-qs-empty branch February 16, 2017 22:56
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty

PR-URL: nodejs#11329
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty

PR-URL: #11329
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
jasnell pushed a commit that referenced this pull request Mar 7, 2017
+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty

PR-URL: #11329
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
+ Add cases to `qs.stringify` that return empty string
+ Add cases to `qs.parse` when `sep` or `eq` is empty

PR-URL: #11329
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
querystring Issues and PRs related to the built-in querystring module. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants